feat(android): [Logs and Metrics Enable Flags 4] Add Timber Logs opt-in - #5943
feat(android): [Logs and Metrics Enable Flags 4] Add Timber Logs opt-in#5943adinauer wants to merge 2 commits into
Conversation
Require an explicit Timber-local opt-in before forwarding Timber messages as Sentry Logs. Add Android options and manifest configuration for auto-installed integrations while preserving event and breadcrumb capture. Co-Authored-By: Claude <noreply@anthropic.com>
|
📲 Install BuildsAndroid
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4f677fb. Configure here.
|
|
||
| if (isTimberAvailable) { | ||
| options.addIntegration(new SentryTimberIntegration()); | ||
| options.addIntegration(new SentryTimberIntegration(options.isEnableTimberLogs())); |
There was a problem hiding this comment.
Timber logs option ignored at init
High Severity
Auto-installed SentryTimberIntegration copies isEnableTimberLogs in its constructor during installDefaultIntegrations, which runs before the SentryAndroid.init configuration callback. Setting enableTimberLogs in that callback therefore never reaches the planted SentryTimberTree, so the Java/Kotlin opt-in silently has no effect. Manifest metadata still works because it is applied earlier.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 4f677fb. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| if (isTimberAvailable) { | ||
| options.addIntegration(new SentryTimberIntegration()); | ||
| options.addIntegration(new SentryTimberIntegration(options.isEnableTimberLogs())); |
There was a problem hiding this comment.
Bug: SentryTimberIntegration is created before the user's configuration callback runs, so it caches the default enableLogs value, ignoring any changes made in the callback.
Severity: MEDIUM
Suggested Fix
The SentryTimberIntegration should not cache the enableLogs value in its constructor. Instead, it should read the value directly from the SentryOptions object whenever it needs to decide whether to capture a log. This ensures that the user's configuration, applied via the callback, is always respected at runtime. The check inside SentryTimberTree should be changed to query options.isEnableTimberLogs instead of using the cached enableLogs field.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location:
sentry-android-core/src/main/java/io/sentry/android/core/AndroidOptionsInitializer.java#L475-L476
Potential issue: The `SentryTimberIntegration` is instantiated during
`AndroidOptionsInitializer.installDefaultIntegrations(...)`, which occurs before the
user's configuration callback is invoked. The integration's constructor caches the value
of `options.isEnableTimberLogs()` at that moment. Consequently, if a user attempts to
enable Timber logging programmatically within the `SentryAndroid.init { options -> ...
}` block, their setting will be silently ignored because the integration has already
been created with the default value (usually `false`). This violates the documented rule
that integrations must not cache option values in their constructors. Only configuration
via `AndroidManifest.xml` works as intended.
Also affects:
sentry-android-timber/src/main/java/io/sentry/android/timber/SentryTimberIntegration.kt:24~30
Did we get this right? 👍 / 👎 to inform future reviews.
1 participant
PR Stack (Logs and Metrics Enable Flags)
📜 Description
Adds an
enableLogsoption toSentryTimberIntegrationandSentryTimberTree, defaulting tofalsewhile preserving their existing JVM constructor signatures.Adds
SentryAndroidOptions.enableTimberLogsand theio.sentry.timber.logs.enabledmanifest key for auto-installed Timber integrations. Manually installed integrations and trees can opt in through their new constructor arguments.Timber forwarding requires both this local opt-in and the existing aggregate core Logs flag until that flag is removed later in the stack. Event and breadcrumb capture remain unchanged.
💡 Motivation and Context
Logging integrations need explicit local opt-ins before the aggregate core Logs flag can be removed. This keeps automatic Timber log forwarding disabled by default while allowing applications to opt in through Java, Kotlin, or Android manifest configuration.
💚 How did you test it?
./gradlew spotlessApply apiDump./gradlew :sentry-android-timber:testReleaseUnitTest :sentry-android-core:testReleaseUnitTest📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
Add the Logcat Logs opt-in before removing the aggregate core Logs flag.